Based on the LDM discussions we should not have separate errors for these violations. The requirement for an implementation is dictated by the presence of an explicit accessibility modifier. That is what mandates their is an implementation, not everything else.

At the same time all of these new scenarios must be predicated on an explicit accessibility modifier. Basically if you see out that is illegal unless there is an explicit accessibility modifier

Fixed

I would consider just grouping all of these messages into one message. Essentially

Both partial method declarations must agree on 'override', 'virtiual', 'sealed' and accessibility

Listing out every single modifier means we have to add a new message here every time we add a feature to the langauge which impacts method signatures.

Will defer to others here if they feel there is more precedent in the other direction.

the existing diagnostics have a distinct error code for 'static' mismatch, 'unsafe' mismatch and 'readonly' mismatch, but I am fine with grouping these together.

Fixed

This is only illegal if the partial method lacks an explicit accessibility modifier.

Fixed

This is only illegal if the partial method lacks an explicit accessibility modifier.

Fixed

This is only illegal if the partial method lacks an explicit accessibility modifier.

Hmm, sounds like these diagnostics will change from "MustHaveImplementation" to "MustHaveAccessibility".

Fixed

Can you explain why this is needed? My mental model is that we already error when partial methods differ an the modifier new. Hence here we're attempting to change the IsNew return to account for illegal code. Is this meant to make the IDE experience cleaner in the face of errors?

I think I have just run myself in a circle on this. My intention was to propose making this legal to support a scenario where a user declares a partial stub which hides a base member, and the generator that produces the implementation doesn't have to think about whether to copy over the 'new' modifier or not. I'll just revert this part.

One question I have: the precedent for misuse of 'new' is to give a warning, not an error. So should the mismatch of 'new' modifiers between parts be a warning or an error?

For now we will simply require identical combinations of 'new' modifiers (giving an error on mismatch). Can revisit if there is a compelling reason to.

Please use Modifier instead of abbreviating as Mod in API surface area.

Fixed

Do we have a test that verifies the DllImportAttribute is emitted correctly in this scenario?

Please also verify the IL for a call to M1() to ensure the call is emitted.

In reply to: 417430836 [](ancestors = 417430836)

Fixed

Feel like there are several categories of tests missing here. Apologies if I read past them:

• partial methods implementing interface members
• partial methods which override members
• partial methods which are both override and have nullability attributes like [MaybeNull] which impact our OHI checks.

have converted your bullet points to checkboxes :)

should it be allowed to specify duplicate nullability attributes? [](start = 26, length = 65)

If we allow nullability attributes on both parts, we should allow attributes on both parts for all attributes.

It is feeling like maybe we should just ship the current rules and see how people feel about it. Adding special allowances for duplicate attributes in partials seems like something you can't just implement speculatively, we have to see how people end up using/experiencing it.

or 'new', or 'extern' [](start = 152, length = 21)

Minor: It looks like there a number of comments with the previous error text.

Fixed.

switch (method1, method2) [](start = 12, length = 25)

Does this switch statement represent a change in behavior from the original if?

Unless there is a change in behavior, we should stick with the if statement since it is significantly simpler. #Closed

Yes, this fixes #43883. This solution ends up creating additional diagnostics in some error scenarios. I tried moving the checks for PartialMethodOnlyOneLatent/PartialMethodOnlyOneActual in here, but because partial method merging already happened in an earlier step, it causes us to miss diagnostics for multiple "actual" methods.

If we could do this at the same time or right before we merge partial methods, we might be able to get higher quality diagnostics in some scenarios when members have conflicting signatures:

1. if the methods are not 'partial', we report ErrorCode.ERR_MemberAlreadyExists
2. if we detect that both methods are partial implementations, we give ErrorCode.ERR_PartialMethodOnlyOneActual
3. if we detect that both methods are partial definitions, we give ErrorCode.ERR_PartialMethodOnlyOneLatent

It might also be possible to fix this by modifying the partial method signature comparer, that groups declarations that might be part of the same partial method, to be aware of RefKind differences.

This solution ends up creating additional diagnostics in some error scenarios.

That sounds ok. We can revisit this later if necessary.

In reply to: 418977822 [](ancestors = 418977822)

CS9051 [](start = 34, length = 6)

There are a number of instances of CS9051.

Fixed

} [](start = 8, length = 1)

Consider testing access outside the type hierarchy as well.

Added more accessibility tests.

Where is there a test where we have a override partial which involves [AllowNull] that succeeds?

I'm not sure I understand. Override_AllowNull_02 has the expected combination of [AllowNull] given on the parameters of the override, and it warns on dereference of the parameters. That seems successful.

Consider an override test where the types change from generic parameters to actual types. Example:

class Base<T> {
  protected virtual void M(T p) { } 
}

partial class Derived : Base<string> {
  protected override partial void M(string p); 
  protected override partial void M(string p) { }
}

Adding to the test plan and will address in a follow-up PR.